Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

tests: add markdown link checker #11358

Merged
merged 25 commits into from
Sep 13, 2020

Conversation

andreizet
Copy link
Contributor

Summary

This is a build related change, it adds a new action that checks for broken links inside markdown files when performing the CI actions.

It will prevent issues like #11089

Related Issues/PRs

@andreizet andreizet requested a review from a team as a code owner September 1, 2020 09:22
@andreizet andreizet requested review from paulirish and removed request for a team September 1, 2020 09:22
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@andreizet andreizet changed the title Added markdown links checker misc(build): added markdown links checker Sep 1, 2020
@andreizet
Copy link
Contributor Author

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Sep 1, 2020
@connorjclark connorjclark changed the title misc(build): added markdown links checker tests: add markdown-link-check Sep 1, 2020
@connorjclark connorjclark changed the title tests: add markdown-link-check tests: add markdown link checker Sep 1, 2020
@connorjclark
Copy link
Collaborator

Looks like we are blocked on tcort/markdown-link-check#94

@connorjclark
Copy link
Collaborator

I like their solution: JabRef/jabref#6695

Would be good to move this to a once-a-week check. Could you change the action to be a cron instead?

Copy link
Contributor Author

@andreizet andreizet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like their solution: JabRef/jabref#6695

Would be good to move this to a once-a-week check. Could you change the action to be a cron instead?

Sure, will do.

@@ -172,3 +172,14 @@ jobs:

- name: Run smoke tests
run: yarn smoke --debug -j=1 --retries=2 dbw oopif offline lantern metrics

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is not the place to be, I can move this to a separate .yml file.

use-quiet-mode: 'yes'
use-verbose-mode: 'yes'
config-file: 'markdown.links.config.json'
folder-path: 'docs/'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there are more files to check, beside the one from docs, we can check them. For all markdown files in the repo, it takes like 11 minutes, and some of them will be marked as 429, since this library has no throttling.

{
"ignorePatterns": [
{
"pattern": "^http://www.tmeter.ru"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TMeter seems to be down, we can remove the link or replace TMeter with something else.

"pattern": "^http://www.tmeter.ru"
},
{
"pattern": "https://sites.google.com/a/chromium.org/chromedriver/capabilities#TOC-perfLoggingPrefs-object"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The url is just fine, but markdown-link-check marks it with 4xx response code. I'm gonna check this with them, and after a fix will be available, we can remove this.

@@ -133,7 +133,7 @@ $ lighthouse --port=9222 --emulated-form-factor=none --throttling.cpuSlowdownMul

## Lighthouse as trace processor

Lighthouse can be used to analyze trace and performance data collected from other tools (like WebPageTest and ChromeDriver). The `traces` and `devtoolsLogs` artifact items can be provided using a string for the absolute path on disk if they're saved with `.trace.json` and `.devtoolslog.json` file extensions, respectively. The `devtoolsLogs` array is captured from the `Network` and `Page` domains (a la ChromeDriver's [enableNetwork and enablePage options]((https://sites.google.com/a/chromium.org/chromedriver/capabilities#TOC-perfLoggingPrefs-object)).
Lighthouse can be used to analyze trace and performance data collected from other tools (like WebPageTest and ChromeDriver). The `traces` and `devtoolsLogs` artifact items can be provided using a string for the absolute path on disk if they're saved with `.trace.json` and `.devtoolslog.json` file extensions, respectively. The `devtoolsLogs` array is captured from the `Network` and `Page` domains (a la ChromeDriver's [enableNetwork and enablePage options](https://sites.google.com/a/chromium.org/chromedriver/capabilities#TOC-perfLoggingPrefs-object)).
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems that a wrongly opened parenthesis was messing up the link. This link was not even transformed by github: https://github.com/GoogleChrome/lighthouse/blob/master/docs/readme.md

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice! this is exactly the kind of thing I wanted to catch.

@andreizet
Copy link
Contributor Author

Looks like we are blocked on tcort/markdown-link-check#94

markdown-link-check needs an option to retry on 429 codes, but it will not help without this dependency update: link-check

@andreizet
Copy link
Contributor Author

andreizet commented Sep 4, 2020

So I've managed to find a solution:
I've splitted the job, so now it checks the files inside /docs, then it checks the ones from root. Also, now it runs every monday at 9 am.

Once the 429 issue will be resolved, I will come back with another pull request, so maybe we can check everything.

https://github.com/andreizet/lighthouse/runs/1070765498?check_suite_focus=true

Copy link
Collaborator

@connorjclark connorjclark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this runs so infrequently, and we've seen it handle all the links, we should be good to merge this without any more changes upstream.

Worse case, we get an email some Mondays with a couple "429s"..

Thanks for taking care of this for us!

This was assigned to @paulirish so I'll defer to him.

@@ -133,7 +133,7 @@ $ lighthouse --port=9222 --emulated-form-factor=none --throttling.cpuSlowdownMul

## Lighthouse as trace processor

Lighthouse can be used to analyze trace and performance data collected from other tools (like WebPageTest and ChromeDriver). The `traces` and `devtoolsLogs` artifact items can be provided using a string for the absolute path on disk if they're saved with `.trace.json` and `.devtoolslog.json` file extensions, respectively. The `devtoolsLogs` array is captured from the `Network` and `Page` domains (a la ChromeDriver's [enableNetwork and enablePage options]((https://sites.google.com/a/chromium.org/chromedriver/capabilities#TOC-perfLoggingPrefs-object)).
Lighthouse can be used to analyze trace and performance data collected from other tools (like WebPageTest and ChromeDriver). The `traces` and `devtoolsLogs` artifact items can be provided using a string for the absolute path on disk if they're saved with `.trace.json` and `.devtoolslog.json` file extensions, respectively. The `devtoolsLogs` array is captured from the `Network` and `Page` domains (a la ChromeDriver's [enableNetwork and enablePage options](https://sites.google.com/a/chromium.org/chromedriver/capabilities#TOC-perfLoggingPrefs-object)).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice! this is exactly the kind of thing I wanted to catch.

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah i also wanted that config file to move out of root. happy that's done. lg!

thanks for working on this @andreizet !

@paulirish paulirish merged commit a585105 into GoogleChrome:master Sep 13, 2020
zhangpeidong-peyton pushed a commit to zhangpeidong-peyton/lighthouse that referenced this pull request Sep 14, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants